Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

More restrictive dependencies for maven #31

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

More restrictive dependencies for maven #31

wants to merge 5 commits into from

Conversation

maybeec
Copy link

@maybeec maybeec commented Jul 3, 2021

Maybe there is a need for including all maven-core, but I couldn't find it in my use case utilizing mmm java maven.
Possibly it's worth thinking about replacing maven-core with maven-model dependency if the rest of maven-core is not needed.

@hohwille
Copy link
Member

hohwille commented Jul 4, 2021

@maybeec thanks for the PR. Minimizing dependencies is always a good thing. I can still extend them later when needed. I am trying to setup CI to verify PRs ind this repo and just added the action to master.

@hohwille
Copy link
Member

hohwille commented Jul 4, 2021

CI issue: #32

hohwille
hohwille requested changes Jul 4, 2021
View reviewed changes
Copy link
Member

@hohwille hohwille left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So adding CI prior to merging was a good step as it reveals that you did not test this change and it breaks the build.

So either we can close this PR or you can rework it so the build is green.

@hohwille hohwille added enhancement SCM software-configuration-management (build, CI, deployment, actions, etc.) labels Jul 4, 2021
@maybeec
Copy link
Author

maybeec commented Jul 4, 2021

should be fixed now

hohwille
hohwille approved these changes Nov 17, 2021
View reviewed changes
Copy link
Member

@hohwille hohwille left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems fine. CI hangs. Trying to restart...

@hohwille
Copy link
Member

@maybeec thanks again for your PR 👍
I would love to merge this.
I can neither edit nor update this feature-branch.
Did you somehow lock your fork of mmm-code?
As the diff is very small, I can also commit the change myself and close this PR, but I would like to understand what is happening in this PR on github for future PRs. All I want is the build to be triggered and see if this change does not break it.

@maybeec
Copy link
Author

maybeec commented Feb 11, 2023

I see it's even not a branch on my fork but on this repository.

@hohwille
Copy link
Member

hohwille commented Feb 13, 2023 •
edited
Loading

Ah, now I get it:

maybeec wants to merge 5 commits into m-m-m:master from maybeec:patch-2

This PR was created from a fork that does not exist anymore. In the early days of Github this broke the PR. Nowadays when a PR is created Github creates an internal branch to keep the diff persistent for the repo.
OK, then I will simply redo the change myself. Sorry that I did not pickup this PR for 1,5 years - what a pity...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hohwille hohwille hohwille approved these changes

Assignees
No one assigned
Labels
enhancement SCM software-configuration-management (build, CI, deployment, actions, etc.)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@maybeec @hohwille